Check slashability of attestations in batches to avoid sequential bottleneck #8516
Conversation
This reverts commit 1fa3b39.
…nto attestation-batch-new
michaelsproul
added
val-client
* WIP refator for safer API * WIP * Refactor * Fmt
|
This isn't really cutting it in terms of performance, even with the change to prune more frequently. I think we might need Rayon to cut the signing times: The previous branch with this + Rayon was working well. |
|
Self-review complete. This is ready for review and hopefully merge 🤞 |
| let single_attestations = safe_attestations | ||
| .iter() | ||
| .zip(validator_indices) | ||
| .zip(validator_indices_ref) |
There was a problem hiding this comment.
validator_indices_ref contains all the attestations to sign, while safe_attestations contains only the safe ones to publish (could be less if there is slashable ones), I think this may not work correctly if the vec sizes don't match?
There was a problem hiding this comment.
maybe the sign_attestations function can return a tuple of attestations and validator_indices? similar to the join_all return values before
| ) -> Result<(), Error> { | ||
| // Make sure the target epoch is not higher than the current epoch to avoid potential attacks. | ||
| if attestation.data().target.epoch > current_epoch { | ||
| return Err(Error::GreaterThanCurrentEpoch { |
There was a problem hiding this comment.
Looks like we no longer need this error variant and cna be deleted
jimmygchen
added
waiting-on-author
2 participants
Issue Addressed
Closes:
Proposed Changes
Sign attestations prior to checking them against the slashing protection DB. This allows us to avoid the sequential DB checks which are observed in traces here:
Additional Info
This PR builds on:
This is a rework of Eitan's PR:
I started by trying to resolve merge conflicts, but there were so many breakages I ended up redoing it. I also left out some of the other changes (like the
AttestationDataService) as we are probably going to introduce a new version of that in the course of implementing the head monitor + consensus service, see: